Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: #110: Explore page #133

Merged
merged 12 commits into from
Nov 27, 2024
Merged

feat: #110: Explore page #133

merged 12 commits into from
Nov 27, 2024

Conversation

VanishMax
Copy link
Contributor

@VanishMax VanishMax commented Nov 14, 2024

Closes #110

The header is ready, trading pair block is only html/css ready. The data is requested and rendered server-side only – all skeletons are removed from the header.

image

@VanishMax VanishMax marked this pull request as ready for review November 25, 2024 12:25
@VanishMax VanishMax requested a review from a team November 25, 2024 12:25
<InfoCard title='Total Trading Volume (24h)'>
<Text large color='text.primary'>
<Text large color='text.primary'>
{shortify(Number(getFormattedAmtFromValueView(stats.directVolume)))}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grod220 the volume comes a 10^21 number, and even the ValueViewComponent displays it as 1589T. Something seems to be wrong in these calculations. Do you know what?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cronokirby need assistance here. directVolume is returning 1444608489888181911552. Assuming this is uusdc, the display exponent is 6. So that gives us 1444608489888181.911552 or 1444T.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also highlight that the liquidity field returns something like liquidity: 118364618673.38, which is not an integer. It requires us to parseInt(), which may fail with big numbers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For mainnet data, I'm only seeing weird values for the month window in the aggregate summary, will investigate, but probably related to needing some more stringent filtering of assets with no real quote asset price on penumbra.

The liquidity field could be amended to not be a float, but every other value that is conceptually an Amount (like volume) is already a float, and so I think I would recommend just developing a bit of infrastructure here to deal with that. We need bespoke code here to deal with prices, which have to be a float, as well.

Comment on lines +42 to +46
// TODO: what asset should be used here?
const usdcMetadata = allAssets.find(asset => asset.symbol.toLowerCase() === 'usdc');
if (!usdcMetadata) {
return { error: 'USDC not found in registry' };
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: the database doesn't seem to have the base asset for the global summary. How ok is it to have this check in code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean quote asset right? @cronokirby think it may be nice for the dex_ex_aggregate_summary table to return the quote asset id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can add metadata for this

Comment on lines +47 to +56
options: {
svgo: false,
svgoConfig: {
plugins: [
{
name: 'preset-default',
params: {
overrides: {
removeViewBox: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allows the import of .svg files as react components. I used it to import PenumbraWaves component.

The removeViewBox: false is needed to keep the dimensions of the svg and scale it without cutting it


export const getStats = async (): Promise<StatsResponse> => {
try {
const durationWindow: DurationWindow = '1d';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I'd pull this above and define it as something like const STATS_DURATION_WINDOW = ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe this should be a query param because I can imagine other duration windows being supported on the stats page in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved as a constant out of the function for now. Supporting query params would also require changing some texts on the page, – not sure we need it for now

Comment on lines +42 to +46
// TODO: what asset should be used here?
const usdcMetadata = allAssets.find(asset => asset.symbol.toLowerCase() === 'usdc');
if (!usdcMetadata) {
return { error: 'USDC not found in registry' };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean quote asset right? @cronokirby think it may be nice for the dex_ex_aggregate_summary table to return the quote asset id.

Comment on lines +77 to +92
return {
activePairs: stats.active_pairs,
trades: stats.trades,
largestPair,
topPriceMover,
largestPairLiquidity:
largestPairEnd &&
toValueView({
amount: stats.largest_dv_trading_pair_volume,
metadata: largestPairEnd,
}),
liquidity: toValueView({
amount: parseInt(`${stats.liquidity}`),
metadata: usdcMetadata,
}),
directVolume: toValueView({ amount: stats.direct_volume, metadata: usdcMetadata }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: We may just be lucky that this is working as you aren't serializing before sending this over the wire. As a ValueView is a class, if you don't serialize/deserialize, the other end may try call a function on the class that doesn't exist. See src/shared/api/server/types.ts for a reference on return types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it doesn't send this over the wire since everything is rendered in react server components with use server (or without use client, to be precise). The data is fetched and immediately transformed to HTML. This valid HTML is then sent over the wire to the client.

<InfoCard title='Total Trading Volume (24h)'>
<Text large color='text.primary'>
<Text large color='text.primary'>
{shortify(Number(getFormattedAmtFromValueView(stats.directVolume)))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cronokirby need assistance here. directVolume is returning 1444608489888181911552. Assuming this is uusdc, the display exponent is 6. So that gives us 1444608489888181.911552 or 1444T.

import { getFormattedAmtFromValueView } from '@penumbra-zone/types/value-view';
import { getStats } from '../api/get-stats';

export const ExploreStats = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea: I kinda think a 2x3 md view is warranted, but I don't see it in figma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call! Added this layout for tablet screens

</InfoCard>
<InfoCard title='Total Liquidity Available'>
<Text large color='text.primary'>
{shortify(Number(getFormattedAmtFromValueView(stats.liquidity)))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why not use ValueViewComponent for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but it there is currently no way to hide the asset icon, and the font sizes differ from the designs

Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we can ship this and have follow ups as needed 👍

@VanishMax VanishMax merged commit 516ff3d into main Nov 27, 2024
2 checks passed
@VanishMax VanishMax deleted the feat/#110-explore-page-header branch November 27, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore page header
3 participants